Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

http: coerce content-length to number #57458

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

http: coerce content-length to number #57458

wants to merge 1 commit into from
+21 −1

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Mar 14, 2025 •
edited
Loading

Fixes: #57456

Either we implictly convert or throw an error. Id would convert it.
We can use one the million tricks to convert it to integer ~~, +, *1 etc... not sure whats consistent with the rest of the project, I found ~~ used often

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Mar 14, 2025
@marco-ippolito marco-ippolito force-pushed the header-content-mismatch branch 3 times, most recently from b82ed47 to e8c4486 Compare March 14, 2025 12:38
@aduh95 aduh95 changed the title http: convert content-length to number http: coerce content-length to number Mar 14, 2025
@aduh95
Copy link
Contributor

aduh95 commented Mar 14, 2025

nit: The ad-hoc term for it in the ES spec is coerce, not convert

@aduh95
Copy link
Contributor

aduh95 commented Mar 14, 2025 •
edited
Loading

Also, ~~(2**31) !== (2**31), not sure it's the right tool here. + would be better.

not sure whats consistent with the rest of the project

Throwing ERR_INVALID_TYPE would be more consistent, but would be semver-major I think

pimterry
pimterry requested changes Mar 14, 2025
View reviewed changes
@codecov Codecov
Copy link

codecov bot commented Mar 14, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.22%. Comparing base (3329efe) to head (da674a9).
Report is 10 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57458      +/-   ##
==========================================
+ Coverage   90.20%   90.22%   +0.02%     
==========================================
  Files         629      629              
  Lines      184948   184937      -11     
  Branches    36204    36214      +10     
==========================================
+ Hits       166837   166867      +30     
+ Misses      11057    11041      -16     
+ Partials     7054     7029      -25
Files with missing lines Coverage Δ
lib/_http_outgoing.js 95.24% <100.00%> (ø)

... and 55 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@marco-ippolito marco-ippolito force-pushed the header-content-mismatch branch from e8c4486 to da674a9 Compare March 14, 2025 14:23
@marco-ippolito marco-ippolito requested a review from pimterry March 14, 2025 14:24
bjohansebas
bjohansebas reviewed Mar 14, 2025
View reviewed changes
Copy link

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zefir-git
Copy link

Throwing ERR_INVALID_TYPE would be more consistent, but would be semver-major I think

That would break ServerResponse#setHeaders(Headers) since Headers values are always string.

@marco-ippolito marco-ippolito added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 14, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65748/

@marco-ippolito marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 14, 2025
lpinca
lpinca reviewed Mar 15, 2025
View reviewed changes
test/parallel/test-http-content-length-mismatch.js
{
const server = http.createServer(common.mustCall((req, res) => {
res.strictContentLength = true;
// Pass content-length as string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move the whole test to a separate file. This more about value type than length mismatch.

mcollina
mcollina approved these changes Mar 15, 2025
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca left review comments

@bjohansebas bjohansebas bjohansebas left review comments

@mcollina mcollina mcollina approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@jasnell jasnell jasnell approved these changes

@pimterry pimterry pimterry approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

http.ServerResponse#strictContentLength = true is broken
10 participants
@marco-ippolito @nodejs-github-bot @aduh95 @zefir-git @mcollina @ShogunPanda @jasnell @lpinca @pimterry @bjohansebas